Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pin golang build to 1.4.2 instead of floating on 1.4 and force pull the base golang image #19847

Merged
merged 2 commits into from
Jan 20, 2016

Conversation

ikehz
Copy link
Contributor

@ikehz ikehz commented Jan 20, 2016

We've experienced some performance regressions since latest golang went from 1.4.2 to 1.4.3. We should pin at a known-good version, at least for now. (#17524)

@ikehz ikehz added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. area/build-release labels Jan 20, 2016
@ikehz
Copy link
Contributor Author

ikehz commented Jan 20, 2016

This should be backported to 1.1 and is cause for a new v1.1.5 release.

@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 20, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/XS

@ikehz ikehz changed the title Pin golang build to 1.4.2 instead of floating on 1.4 Pin golang build to 1.4.2 instead of floating on 1.4 and force pull the base golang image Jan 20, 2016
@@ -490,16 +490,19 @@ function kube::build::build_image_cross() {
local -r build_context_dir="${LOCAL_OUTPUT_ROOT}/images/${KUBE_BUILD_IMAGE}/cross"
mkdir -p "${build_context_dir}"
cp build/build-image/cross/Dockerfile ${build_context_dir}/Dockerfile
kube::build::docker_build "${KUBE_BUILD_IMAGE_CROSS}" "${build_context_dir}"
kube::build::docker_build "${KUBE_BUILD_IMAGE_CROSS}" "${build_context_dir}" 'true'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why single quotes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and is there a reason to leave the one on L483 as false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why single quotes?

Double-quotes indicates substitution intended, single-quotes indicates literal.

and is there a reason to leave the one on L483 as false?

Yes, if we try to force-pull image that only exists locally (i.e. the image that the one on L483 is built on) Docker complains that it can't pull the image. I will add a note. Or maybe I should default to true?

@k8s-bot
Copy link

k8s-bot commented Jan 20, 2016

GCE e2e test build/test passed for commit e95dd01.

@k8s-bot
Copy link

k8s-bot commented Jan 20, 2016

GCE e2e test build/test passed for commit 5a383ed6ac538bc2df0d5f121cba2851425211d4.

@k8s-bot
Copy link

k8s-bot commented Jan 20, 2016

GCE e2e test build/test passed for commit 09b2cc8a0f21bc1ce041d08cc73ee2cf9934657c.

@zmerlynn
Copy link
Member

LGTM once you address @ixdy's comments

@ikehz
Copy link
Contributor Author

ikehz commented Jan 20, 2016

@ixdy @zmerlynn PTAL.

@k8s-github-robot
Copy link

Labelling this PR as size/S

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 20, 2016
@k8s-bot
Copy link

k8s-bot commented Jan 20, 2016

GCE e2e build/test failed for commit 041f152.

kube::build::docker_build "${KUBE_BUILD_IMAGE}" "${build_context_dir}"
# We don't want to force-pull this image because it's based on a local image
# (see kube::build::build_image_cross), not upstream.
kube::build::docker_build "${KUBE_BUILD_IMAGE}" "${build_context_dir}" 'false'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, that's obvious. Thanks.

@zmerlynn zmerlynn added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2016
@zmerlynn
Copy link
Member

@k8s-bot e2e test this

@k8s-bot
Copy link

k8s-bot commented Jan 20, 2016

GCE e2e build/test failed for commit 041f152.

@ikehz
Copy link
Contributor Author

ikehz commented Jan 20, 2016

@k8s-bot e2e test this again please

@ixdy
Copy link
Member

ixdy commented Jan 20, 2016

LGTM2

@k8s-bot
Copy link

k8s-bot commented Jan 20, 2016

GCE e2e test build/test passed for commit 041f152.

@ikehz
Copy link
Contributor Author

ikehz commented Jan 20, 2016

@k8s-oncall can we merge this manually to get the tests stable again?

@ikehz
Copy link
Contributor Author

ikehz commented Jan 20, 2016

(@k8s-oncall we also need to backport this to release-1.1)

@ixdy
Copy link
Member

ixdy commented Jan 20, 2016

Also release-1.0 (if we still care about that?)

@zmerlynn
Copy link
Member

@ihmccreery: Er, when did cherry picks shift to @k8s-oncall's responsibility?

@ikehz
Copy link
Contributor Author

ikehz commented Jan 20, 2016

@ihmccreery: Er, when did cherry picks shift to @k8s-oncall's responsibility?

Oh, no, I didn't mean it was their responsibility; just that we should get this merged asap so I can cherrypick it.

alex-mohr added a commit that referenced this pull request Jan 20, 2016
Pin golang build to 1.4.2 instead of floating on 1.4 and force pull the base golang image
@alex-mohr alex-mohr merged commit 74d3506 into kubernetes:master Jan 20, 2016
alex-mohr added a commit that referenced this pull request Jan 20, 2016
…847-upstream-release-1.1

Automated cherry pick of #19847
alex-mohr added a commit that referenced this pull request Jan 22, 2016
…847-upstream-release-1.0

Automated cherry pick of #19847
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
…ick-of-#19847-upstream-release-1.1

Automated cherry pick of kubernetes#19847
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
…ick-of-#19847-upstream-release-1.1

Automated cherry pick of kubernetes#19847
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build-release lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants